Skip to content

feat(track-changes): block-level structural tracked changes for tables#3343

Open
shri-scale wants to merge 24 commits into
superdoc-dev:mainfrom
shri-scale:feature/block-level-tracked-changes
Open

feat(track-changes): block-level structural tracked changes for tables#3343
shri-scale wants to merge 24 commits into
superdoc-dev:mainfrom
shri-scale:feature/block-level-tracked-changes

Conversation

@shri-scale

Copy link
Copy Markdown
Contributor

Summary

Adds a new StructuralTrackChanges extension that gives block-level (table) add/remove operations the same review pipeline as inline tracked changes — same data-track-change rendering, same accept/reject command surface, plus block-level entry registration in the comments-plugin so the review UI surface has the data it needs.

  • Consumer computes structural hunks (computeStructuralDiff or their own logic) and dispatches via editor.commands.setStructuralDiff(hunks).
  • Extension stamps a trackChange PM attribute on each affected tableRow; painter reads it and stamps data-track-change* on cells; CSS styles them.
  • Existing Diffing, compareDocuments, replayDifferences, and inline TrackChanges are untouched. New extension is opt-in via editorExtensions: [StructuralTrackChanges] (not in starter extensions).

What's in this PR

Layer Change
Schema Shared blockTrackedChangeAttrSpec helper; spread into TableRow.addAttributes
Contracts TrackedChangeMeta.operationId? + TableRow.trackedChange?
pm-adapter parseTableRow populates TableRow.trackedChange from the PM attr
painter-dom renderTableRow stamps data-track-change* on each cell of a tracked row
CSS [data-track-change="insert" | "delete"] rules, themable via --sd-block-tracked-*
Helpers getBlockTrackedChanges, applyRowTrackedChangeResolution (in track-changes)
Extension StructuralTrackChanges with setStructuralDiff, accept/reject by id, bulk, and operation-grouped commands
acceptTrackedChangeById / rejectTrackedChangeById Extended to also route row-attr ids and operationIds (falls back through 3 paths: inline mark → row id → operationId)
trackedTransaction interceptor Passes through ReplaceSteps whose slice already carries block-level trackChange attrs (no double-tracking)
Comments-plugin Walks block-level entries alongside inline marks; populates pluginState.trackedChanges and allCommentPositions
Public API StructuralTrackChanges, computeStructuralDiff re-exported from the v1 barrel

Test plan

  • Run full super-editor suite: pnpm test from packages/super-editor/ — should be green (~13,000 tests).
  • Targeted suites: pnpm vitest run blockTrackedChangeAttr getBlockTrackedChanges acceptRejectRowTrackedChange blockTrackedChangePassthrough computeStructuralDiff applyHunks structural-track-changes blockTrackedChangeBubble
  • End-to-end fixture: pnpm vitest run structural-track-changes-e2e — exercises computeStructuralDiffsetStructuralDiffacceptAllStructuralChanges / rejectAllStructuralChanges against a real .docx pair.
  • Manual smoke (consumer pattern): in a SuperDoc instance with editorExtensions: [StructuralTrackChanges], dispatch a remove hunk via setStructuralDiff → the matching table's rows turn red. acceptAllStructuralChanges removes the table.

Out of scope (separate follow-ups)

  • DOCX round-trip of block-level tracked changes (the schema attr survives through PM renderDOM/parseDOM, but no OOXML writer/reader is wired)
  • Block-level entries surfacing in the default SuperDoc bubble. Plugin state has the entries, but inline tracked changes' bubble appears via an editor.emit('commentsUpdate', ...) payload pipeline that block-level changes don't yet emit. Consumers with custom review UI (al-pmo today) can wire from getBlockTrackedChanges directly.
  • Cell-level / column-level tracking
  • A combined acceptAllChanges that batches inline + structural into one transaction (currently two sequential commands; two undo steps)
  • Smarter structural-fingerprint matcher for computeStructuralDiff when sdBlockIds differ across imports (consumer can supply a custom identityKey)

🤖 Generated with Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b9d84246ae

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread packages/super-editor/src/editors/v1/extensions/comment/comments-plugin.js Outdated
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@shri-scale

Copy link
Copy Markdown
Contributor Author

Hi @caio-pizzol, I know its big change, just kind request

@shri-scale shri-scale force-pushed the feature/block-level-tracked-changes branch from 2b176d2 to 48bdef8 Compare May 20, 2026 10:57

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 38 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/super-editor/src/editors/v1/extensions/track-changes/blockTrackedChangeAttr.js">

<violation number="1" location="packages/super-editor/src/editors/v1/extensions/track-changes/blockTrackedChangeAttr.js:13">
P2: Return `null` from `parseDOM` when `data-track-change-id` is absent. A tracked change without an id cannot be resolved by `acceptRejectRowTrackedChange` (which matches by id), leaving an unactionable entry in the document. Guarding on `id` also keeps the runtime shape consistent with the documented type (`id: string` when non-null).</violation>
</file>

<file name="packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/trackedTransaction.js">

<violation number="1" location="packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/trackedTransaction.js:381">
P1: Missing `map.appendMap(step.getMap())` after applying the step. In multi-step transactions, subsequent steps will be mapped using stale positions that don't account for the document changes made by this block-level step, potentially causing position corruption or silently dropped steps.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

@shri-scale

Copy link
Copy Markdown
Contributor Author

Hi @caio-pizzol , any idea when this will be reviewed? Appreciate if it can be expedited. thanks

shri-scale added a commit to shri-scale/superdoc that referenced this pull request Jun 15, 2026
…tchet

Upstream's JSDoc ratchet (`packages/superdoc/scripts/check-jsdoc.cjs`)
requires every new public-reachable JSDoc file to either carry
`// @ts-check` or be on the allowlist. Our two new files
(`applyHunks.js`, `computeStructuralDiff.js`) tripped the gate and
broke the build stage on PR superdoc-dev#3343.

- Add `// @ts-check` to both files.
- Add a `StructuralHunk` typedef in applyHunks.js so the hunk array's
  properties (`changeId`, `kind`, `basePos`, `anchorBasePos`,
  `proposalNode`) are typed instead of the previous opaque `object[]`.
- Narrow `basePos` / `anchorBasePos` from `number | undefined` to
  `number` via runtime guards so the `tr.mapping.map(...)` calls
  type-check.
@shri-scale shri-scale force-pushed the feature/block-level-tracked-changes branch from f051945 to 03256f0 Compare June 15, 2026 11:56
shri-scale added a commit to shri-scale/superdoc that referenced this pull request Jun 15, 2026
…tchet

Upstream's JSDoc ratchet (`packages/superdoc/scripts/check-jsdoc.cjs`)
requires every new public-reachable JSDoc file to either carry
`// @ts-check` or be on the allowlist. Our two new files
(`applyHunks.js`, `computeStructuralDiff.js`) tripped the gate and
broke the build stage on PR superdoc-dev#3343.

- Add `// @ts-check` to both files.
- Add a `StructuralHunk` typedef in applyHunks.js so the hunk array's
  properties (`changeId`, `kind`, `basePos`, `anchorBasePos`,
  `proposalNode`) are typed instead of the previous opaque `object[]`.
- Narrow `basePos` / `anchorBasePos` from `number | undefined` to
  `number` via runtime guards so the `tr.mapping.map(...)` calls
  type-check.
@shri-scale

Copy link
Copy Markdown
Contributor Author

HI @caio-pizzol Its been month this PR been up, rebasing/merging is tricky, and may infact cause more issues. Can you please expedite or direct on this PR ?

shri-scale added a commit to shri-scale/superdoc that referenced this pull request Jun 18, 2026
…tchet

Upstream's JSDoc ratchet (`packages/superdoc/scripts/check-jsdoc.cjs`)
requires every new public-reachable JSDoc file to either carry
`// @ts-check` or be on the allowlist. Our two new files
(`applyHunks.js`, `computeStructuralDiff.js`) tripped the gate and
broke the build stage on PR superdoc-dev#3343.

- Add `// @ts-check` to both files.
- Add a `StructuralHunk` typedef in applyHunks.js so the hunk array's
  properties (`changeId`, `kind`, `basePos`, `anchorBasePos`,
  `proposalNode`) are typed instead of the previous opaque `object[]`.
- Narrow `basePos` / `anchorBasePos` from `number | undefined` to
  `number` via runtime guards so the `tr.mapping.map(...)` calls
  type-check.
@shri-scale shri-scale force-pushed the feature/block-level-tracked-changes branch from 4328183 to cf1189a Compare June 18, 2026 08:18
@caio-pizzol

caio-pizzol commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Hi @shri-scale, first, thank you for this, and I'm sorry it sat so long. This is a big, careful change, especially the test coverage. The honest reason for the delay is that it touches always-on paths: schema, comments plugin, painter, layout cache, and accept/reject. That meant we needed to verify the OOXML model and look closely at how this fits with what already exists in the editor.

The direction is right: OOXML represents inserted/deleted table rows with w:ins / w:del under w:trPr, with no table-level insert/delete marker. That matches the row-level model here.

The main issue is architectural: we already have a structural-row pipeline: enumerateStructuralRowChanges, the review graph, stampTableRows, and importer/exporter support in row-track-change.js. This PR adds a second model beside it: getBlockTrackedChanges, operationId, and applyHunks. Two parallel models can disagree. For example, an imported multi-row deleted table has per-row Word ids and no operationId, so the existing pipeline groups it as one structural table change while the new block walk can see separate row entries. Before merge, we'd want this built on the existing structural-row pipeline rather than beside it.

Concrete items:

  1. In table-row.js, ...blockTrackedChangeAttrSpec is spread, then trackChange is redefined later with rendered: false. That means the helper's parse/render behavior is dead for table rows, so the DOM round-trip described in the PR does not actually happen for rows.

  2. Please stamp/generated structural changes through the existing path, or otherwise align with it, so generated revisions carry the same metadata shape as native tracked table revisions: author/date/revision grouping and valid export behavior.

  3. acceptAllTrackedChanges / rejectAllTrackedChanges currently return reviewDecision.applied, so a block-only change can mutate the document but report false.

  4. The PR description is stale in two places: the extension is registered in getStarterExtensions(), and row-level OOXML reader/writer support already exists through the converter.

  5. Paragraph support looks like a separate design question. OOXML paragraph-mark revisions are not the same primitive as table-row insert/delete revisions, so I'd prefer splitting that into its own PR with its own justification.

Happy to pair on the pipeline reconciliation. I don't think we should set a merge ETA until the structural-row integration direction is settled, but once that revision is up and CI is green, we should be able to do a focused re-review quickly.

For faster comms, let's try to connect on the side (discord, caio@superdoc.dev) - so make sure to give this the priority it needs

shri-scale and others added 5 commits June 22, 2026 22:12
Adds a new StructuralTrackChanges extension that gives block-level
(table) add/remove operations the same review pipeline as inline tracked
changes - same data-track-change rendering, same accept/reject command
surface, plus block-level entry registration in the comments-plugin.

Consumer pattern: compute structural hunks via computeStructuralDiff (or
construct them yourself), dispatch via editor.commands.setStructuralDiff
(hunks). The extension stamps a trackChange PM attribute on each affected
tableRow. Rendering is handled by the painter (reads row.trackedChange,
stamps data-track-change* on cells). Accept/reject operates on PM attrs
via getBlockTrackedChanges + applyRowTrackedChangeResolution.

Pipeline pieces:
- Shared blockTrackedChangeAttrSpec helper + TableRow.addAttributes spread
- TableRow.trackedChange contract field
- pm-adapter populates TableRow.trackedChange from PM attr
- painter renderTableRow stamps data-track-change on cells
- CSS rules for [data-track-change="insert" | "delete"]
- getBlockTrackedChanges walks the doc for block-level entries
- applyRowTrackedChangeResolution handles accept/reject by id
- acceptTrackedChangeById / rejectTrackedChangeById extended to route
  inline marks, row attrs, or operationId
- trackedTransaction passes through ReplaceSteps that already carry
  block-level metadata so inline marks don't double-track
- comments-plugin walks block-level entries alongside inline marks

Existing Diffing extension, compareDocuments, replayDifferences, and
inline TrackChanges are untouched. The new extension is opt-in via
editorExtensions: [StructuralTrackChanges] (not in starter extensions).

Tests: ~30 new unit tests across the touched modules, plus an
end-to-end test using a real .docx fixture pair that exercises
compute -> set -> accept-all and compute -> set -> reject-all.

Out of scope (separate follow-ups):
- DOCX round-trip of block-level tracked changes
- Block-level entries surfacing in the default SuperDoc bubble (requires
  mirroring inline's commentsUpdate event payload pipeline)
- Cell-level / column-level tracking
- A combined acceptAllChanges that batches inline + structural into one
  transaction (currently two sequential commands; two undo steps)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed changes

- Move painter-targeting CSS into BLOCK_TRACK_CHANGE_STYLES in the painter's
  styles module so document visuals live behind the painter boundary; keep
  contenteditable-side rules scoped under .sd-editor-scoped .ProseMirror tr.
- Drop stale isBlockLevel entries from previous state before merging in
  freshly-computed block entries, so accept/reject clears the bubble.
- renderDOM now emits data-track-change-id and data-track-change-operation
  alongside data-track-change so HTML round-trips preserve enough state for
  getBlockTrackedChanges and acceptTrackedChangeById to resolve the row.

Adds regression tests: stale-entry pruning in comments-plugin apply(),
renderDOM/parseDOM round-trip in blockTrackedChangeAttr, and a styles.ts
guard that fails if a bare [data-track-change=...] selector regresses.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ensions

Consumers (Superdoc Vue host, @superdoc-dev/react wrapper) take their
extension list from getStarterExtensions(); without this entry the new
block-level extension was exported but never loaded into the editor.

Updates the dedicated test that previously asserted opt-in-only behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The comments-plugin apply() reducer walked the entire doc for block-level
tracked rows on every tr.docChanged, even on docs that had no tracked rows.
Combined with in-place mutation of pluginState.trackedChanges and the
view.update decoration rebuild path, this created a stream of state
references that downstream consumers reacted to, contributing to focus
steal in side-by-side layouts where the editor sat next to an unrelated
input control.

Two structural changes:

- Track a hasBlockChanges bit on plugin state, computed once at init and
  refreshed only when a walk runs. Gate the block walk on
  (hasBlockChanges OR inputType='acceptReject' meta) so typing in a doc
  with no tracked rows never triggers the walk.
- Stop mutating pluginState.trackedChanges and pluginState.activeThreadId
  inside apply(); compute next-values locally and return them via the
  spread at the end. Plugin state must be treated as immutable across
  apply() per ProseMirror convention.

Adds regression tests that lock the gate (hasBlockChanges stays false
through typing, flips true once a tracked row appears).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed editors

Track which DOMs the bridge has owned and only redirect keystrokes to a
stale ProseMirror[contenteditable=true] candidate if it is in that set.
Without this guard, an unrelated PM-based editor (Tiptap, Remirror)
mounted alongside SuperDoc would receive redirected input.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shri-scale and others added 16 commits June 22, 2026 22:12
…nd export structural API

acceptAllTrackedChanges and rejectAllTrackedChanges now also dispatch
acceptAllStructuralChanges / rejectAllStructuralChanges when block-level
row entries exist, so the single accept-all/reject-all surface resolves
both inline marks and table-row tracked changes. Without this, a
table-delete operation would leave the row, cell, and table shell in
place after accept-all.

Also re-export StructuralTrackChanges and computeStructuralDiff from the
superdoc package entry so SDK consumers can wire up structural diffs
without importing from super-editor directly.

git commit -F /tmp/cmsg2.txt
The WeakSet-based ownership check broke header/footer/footnote/endnote
accept/reject + undo (6 behavior tests in story-surface-tracked-change-decide):
story editors weren't registered in the bridge's owned set when sidebar
operations were the first interaction, so the bridge dropped the
stale-target redirect that those flows depend on.

Switch the check to a class-based ancestor lookup
(`closest('.sd-editor-scoped')`). Every SuperDoc editor — including
header/footer/footnote/endnote — is rendered inside a `.sd-editor-scoped`
container, so they're recognized without needing prior bridge activation;
foreign PM editors (Tiptap, Remirror, raw PM) are not, so the focus-steal
fix is preserved.

Bridge unit tests updated to wrap stale editors in a `.sd-editor-scoped`
ancestor (matches production DOM); foreign-editor regression test
unchanged (its `.tiptap.ProseMirror` element has no SuperDoc ancestor and
is correctly ignored).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds StructuralTrackChanges + computeStructuralDiff to the SD-3176
legacy-exports snapshot (intentional growth on superdoc/super-editor).
The named export is what consumers will import via:
  import { StructuralTrackChanges, computeStructuralDiff } from "superdoc";

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When insertTrackedChange is called with from === to at a position
where bare text can't sit (e.g. at doc end past the last block), PM
auto-wraps the inserted text in the schema's required parents
(paragraph + run). The previous mark range used insertedNode.nodeSize
-- the bare text length -- which then covered the wrapper's open
tokens instead of the trailing characters of the actual text. Those
trailing chars escaped the trackInsert mark and survived accept/reject
as orphan text nodes (ALPMO-245).

Capture the doc size delta around the insert step and use it as the
mark range. addMark only attaches to text nodes inside the range, so
marking wrapper boundaries is a no-op there but pulls in the trailing
chars that PM's auto-wrap pushed out of the bare-nodeSize window.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Block-level diff replay sets `trackedChange` on a tableRow via the
StructuralTrackChanges extension. The three caches that drive painted
output -- the painter's per-page fingerprint in renderer.ts, the
measure cache in layout-bridge, and the canonical block version in
layout-resolved -- previously hashed only the cells, missing the
row-level attribute.

Result: applyHunks-style transactions that only mutated
`row.trackChange` reused stale cache entries, the page never
re-measured or repainted, and visible cells never received their
`data-track-change` attribute (so the row stayed unmarked until a
full scroll forced a remeasure).

Mirror the row.trackedChange fingerprint into all three caches so the
same transaction invalidates each one.
…print

computeStructuralDiff previously matched top-level blocks by
sdBlockId. That works in-process but breaks across imports: the docx
importer assigns a fresh sdBlockId on every load (there's no standard
OOXML attribute for tables to carry a stable id), so the AI-review
flow -- where the proposal is a separately-imported docx -- saw every
block as new and flagged the whole document as changed.

Switch the default identityKey to a normalized content fingerprint
(`${typeName}:${normalizedText}`) so identical blocks match across
imports. Document the limitations in the JSDoc: two truly-identical
blocks share a fingerprint and the algorithm treats them as one
(acceptable for current AI-review flow; consumers needing precise
duplicate handling can pass a custom `identityKey`). Consumers whose
proposal preserves sdBlockIds can opt back in via the option.
The block-level tracked-change shortcut in trackedTransaction applied
the original step as-is via `newTr.step(step)` but didn't call
`map.appendMap(step.getMap())`. In a multi-step transaction where this
shortcut fires before another step, the next iteration's
`originalStep.map(map)` then mapped through stale positions -- the
later step landed in the wrong place or got silently dropped when the
mapping returned null.

Matches the existing pass-through pattern in replaceStep.js where
applying a step as-is is always paired with `map.appendMap`. The
StructuralTrackChanges applyHunks path typically dispatches one step
per transaction, so the bug rarely hit today, but it becomes a real
correctness issue the moment two structural hunks share a transaction
or any other tracked change rides alongside a structural one.
`blockTrackedChangeAttrSpec.parseDOM` accepted nodes with
`data-track-change` set but `data-track-change-id` missing, producing
a `trackChange: { kind, id: null, operationId }` shape on the PM
node. That shape:

- can never be resolved by `applyRowTrackedChangeResolution`, which
  matches by id and would never find a null
- is already filtered out by `getBlockTrackedChanges` (so the entry
  is unactionable, just lingering as a no-op attr on the row)
- violates the documented `id: string` runtime type
- is asymmetric with `renderDOM`, which only emits
  `data-track-change-id` when `tc.id` is truthy — a faithful HTML
  round-trip always carries the id

Reject the missing-id case at parse time so the doc never holds a
half-formed trackChange shape.
…rmat

Upstream main introduced an OOXML-aligned shape for row tracked changes
(`attrs.trackChange.type === 'rowInsert' | 'rowDelete'`) and a
`buildRowTrackedChangeMeta` reader on the v1 layout-adapter that writes
the resolved metadata to `row.attrs.trackedChange`. Several of our
block-level pieces still spoke the older `{ kind }` shape and wrote to
the top-level `row.trackedChange`, so after the rebase onto main the
producer/consumer pair disagreed and painting silently no-oped:

  - `applyHunks` stamped `{ kind, id, operationId }` on the PM tableRow.
    The new reader (which keys on `type`) ignored it, so
    `row.attrs.trackedChange` was never populated. Switched to
    `{ type: 'rowInsert' | 'rowDelete', id, operationId, ... }`. Tests
    updated.
  - `comments-plugin` block-walk and `getBlockTrackedChanges` accepted
    only `{ kind }`. They now accept both shapes and emit the same
    normalized `kind` downstream.
  - `trackedTransaction.sliceContainsPreMarkedBlockTrackedChange`
    looked for `kind`; broadened to also detect `type === 'rowInsert' |
    'rowDelete'` so paste/replay slices skip double-tracking under
    either shape.
  - The legacy fallback path in `table.ts` (v1 layout-adapter) that
    populated the top-level `row.trackedChange` from the old shape is
    removed; `buildRowTrackedChangeMeta` is now the single producer.
  - The painter page-fingerprint, layout-bridge measure-cache, and
    layout-resolved canonical block version now hash
    `row.attrs.trackedChange` instead of the (now-unpopulated) top-level
    field, so applyHunks-style transactions invalidate the right caches.
  - `structural-track-changes` JSDoc refreshed to describe the new flow.

Net effect: a block-level diff replay now reliably paints the red/green
row decoration via the helper-based path (`applyRowTrackedChangeToCell`)
that upstream extracted.
Commit fff5326 ("invalidate row caches on tableRow.trackChange attr
changes") accidentally added a 738-line block to the bottom of the
painter's renderer.ts — a duplicate `deriveBlockVersion` implementation
plus supporting helpers (`getSdtMetadataId`, `hasListMarkerProperties`,
`hasVerticalPositioning`, etc.) that referenced names (`SdtMetadata`,
`TableAttrs`, `TextRun`, `ParagraphAttrs`, `getRunBooleanProp`,
`hashTableBorders`, `normalizeBaselineShift`, `applyRtlStyles`, …) which
were never imported into renderer.ts. The block was never called from
outside (`grep` confirms no external `deriveBlockVersion` reference in
the painter package), and the canonical `deriveBlockVersion` lives in
`layout-resolved/src/versionSignature.ts` which already has the
row-attrs.trackedChange hash from the audit commit. So the only effect
of the orphan block was to break `tsc -b` and the upstream CI build.

Truncate renderer.ts back to its upstream-main shape (last meaningful
line: `isNonBodyStoryBlockId`). The row-level cache invalidation
continues to work via `versionSignature.ts` and `layout-bridge/cache.ts`
(which read `row.attrs.trackedChange` correctly).
…tchet

Upstream's JSDoc ratchet (`packages/superdoc/scripts/check-jsdoc.cjs`)
requires every new public-reachable JSDoc file to either carry
`// @ts-check` or be on the allowlist. Our two new files
(`applyHunks.js`, `computeStructuralDiff.js`) tripped the gate and
broke the build stage on PR superdoc-dev#3343.

- Add `// @ts-check` to both files.
- Add a `StructuralHunk` typedef in applyHunks.js so the hunk array's
  properties (`changeId`, `kind`, `basePos`, `anchorBasePos`,
  `proposalNode`) are typed instead of the previous opaque `object[]`.
- Narrow `basePos` / `anchorBasePos` from `number | undefined` to
  `number` via runtime guards so the `tr.mapping.map(...)` calls
  type-check.
…o root exports

Surgical update to satisfy the export-snapshots gate (SD-3212 A0). CI
flagged drift in the runtime root exports — the two new named exports
from the structural-track-changes feature need to be in the snapshot's
import + require name lists.
Last commit's replace_all match also inserted computeStructuralDiff into
the types.import / types.require lists. It's a function (runtime export
only — has no separate type export), so the public-facade types facade
doesn't re-export it. Removing the type entries; the runtime
import + require entries stay.
…lass-based selectors

Upstream replaced the previous `[data-track-change='insert'|'delete']`
attribute selectors with class-based decoration (`.track-row-cell-dec
.track-insert-dec.highlighted` etc.) emitted by the new
`applyRowTrackedChangeToCell` helper. Our scoped-selector regression
test still asserted the old attribute form, so it tripped the unit
suite after rebase. Updated to assert the new row-cell class selectors
are scoped to `.superdoc-layout` — preserves the original intent (no
leaks into the PM mirror) against the current CSS surface.
Generalize the table-row block-tracking path to whole paragraphs so a
paragraph insert/delete is a reviewable structural tracked change
("remove the bullet points" -> reviewable paragraph deletions):

- paragraph schema spreads blockTrackedChangeAttrSpec
- applyHunks stamps a paragraph node directly, not just tableRow children
- acceptRejectRowTrackedChange resolves a top-level block (depth 0) without
  the $pos.before(0) "no position before the top-level node" crash
- layout-adapter projects the paragraph trackChange; DomPainter strikes the
  whole paragraph block
…tructural-row pipeline

PR-review feedback: this branch was building a parallel block-level
infrastructure (getBlockTrackedChanges, applyRowTrackedChangeResolution,
blockTrackedChangeAttrSpec, operationId, `{kind}` row attr shape) alongside
the existing structural-row pipeline (stampTableRows,
enumerateStructuralRowChanges, review-graph, decision-engine,
row-track-change.js OOXML import/export). Two parallel models could
disagree: an imported multi-row deleted table groups as one structural
change in the existing pipeline but as separate row entries in the new
one. Per ALPMO-... feedback, route everything through the existing
pipeline.

Changes:

- applyHunks now delegates to stampTableRows(). Same attribute shape
  ({ type: 'rowInsert' | 'rowDelete', id, author, authorId, authorEmail,
  authorImage, date, revisionGroupId }) end-to-end: OOXML import →
  enumerator → review-graph → decision-engine → OOXML export. The
  consumer (al-pmo) keeps its setStructuralDiff(hunks) entry point; the
  body just threads editor.options.user and a fresh ISO timestamp into
  applyHunks.

- structural-track-changes extension trimmed to one command,
  setStructuralDiff. acceptStructuralChange / rejectStructuralChange /
  acceptAllStructuralChanges / rejectAllStructuralChanges /
  acceptTrackedChangeOperation / rejectTrackedChangeOperation deleted —
  the review-graph projects whole-table structural changes into the
  unified decision model, so acceptTrackedChangeById /
  acceptAllTrackedChanges / their reject twins already cover this via
  dispatchReviewDecision.

- accept/rejectTrackedChangeById and accept/rejectAllTrackedChanges drop
  their block-level fallbacks. The "returns false on block-only changes"
  bug the reviewer flagged disappears with the fallback — we now just
  return reviewDecision.applied, which is true because the review-graph
  feeds structural changes into the decision engine.

- table-row.js dead-code bug (reviewer-flagged): the
  ...blockTrackedChangeAttrSpec spread was shadowed by the canonical
  trackChange definition below it, so the helper's parse/render were
  dead. Spread removed.

- paragraph.js: same spread removed. Paragraph-level structural revisions
  are an OOXML paragraph-mark primitive (w:rPr/w:ins), distinct from
  row insert/delete — per reviewer, split into a separate PR with its
  own design justification.

- comments-plugin block-bubble walk now consumes
  enumerateStructuralRowChanges (one entry per whole-table change keyed
  by stable structural id), replacing the manual descend that grouped
  by operationId. Same applies to the view-decoration handler.

- trackedTransaction pre-marked detector reads only the upstream type
  shape ('rowInsert' | 'rowDelete'); the legacy { kind } branch is gone
  because no producer writes that shape anymore.

- Public surface: enumerateStructuralRowChanges re-exported from
  @scale/superdoc so consumers (al-pmo) can iterate logical structural
  changes by their stable public id.

Deletes blockTrackedChangeAttr.js, getBlockTrackedChanges.js,
acceptRejectRowTrackedChange.js, blockTrackedChangeBubble.test.js, and
their tests. Net ~600 lines removed.
@shri-scale shri-scale force-pushed the feature/block-level-tracked-changes branch from cf1189a to daccb98 Compare June 23, 2026 10:48
@shri-scale

Copy link
Copy Markdown
Contributor Author

Hi @shri-scale, first, thank you for this, and I'm sorry it sat so long. This is a big, careful change, especially the test coverage. The honest reason for the delay is that it touches always-on paths: schema, comments plugin, painter, layout cache, and accept/reject. That meant we needed to verify the OOXML model and look closely at how this fits with what already exists in the editor.

The direction is right: OOXML represents inserted/deleted table rows with w:ins / w:del under w:trPr, with no table-level insert/delete marker. That matches the row-level model here.

The main issue is architectural: we already have a structural-row pipeline: enumerateStructuralRowChanges, the review graph, stampTableRows, and importer/exporter support in row-track-change.js. This PR adds a second model beside it: getBlockTrackedChanges, operationId, and applyHunks. Two parallel models can disagree. For example, an imported multi-row deleted table has per-row Word ids and no operationId, so the existing pipeline groups it as one structural table change while the new block walk can see separate row entries. Before merge, we'd want this built on the existing structural-row pipeline rather than beside it.

Concrete items:

  1. In table-row.js, ...blockTrackedChangeAttrSpec is spread, then trackChange is redefined later with rendered: false. That means the helper's parse/render behavior is dead for table rows, so the DOM round-trip described in the PR does not actually happen for rows.
  2. Please stamp/generated structural changes through the existing path, or otherwise align with it, so generated revisions carry the same metadata shape as native tracked table revisions: author/date/revision grouping and valid export behavior.
  3. acceptAllTrackedChanges / rejectAllTrackedChanges currently return reviewDecision.applied, so a block-only change can mutate the document but report false.
  4. The PR description is stale in two places: the extension is registered in getStarterExtensions(), and row-level OOXML reader/writer support already exists through the converter.
  5. Paragraph support looks like a separate design question. OOXML paragraph-mark revisions are not the same primitive as table-row insert/delete revisions, so I'd prefer splitting that into its own PR with its own justification.

Happy to pair on the pipeline reconciliation. I don't think we should set a merge ETA until the structural-row integration direction is settled, but once that revision is up and CI is green, we should be able to do a focused re-review quickly.

For faster comms, let's try to connect on the side (discord, caio@superdoc.dev) - so make sure to give this the priority it needs

Hi @caio-pizzol — thanks for the careful review, you were right about the parallel model. Pushed a refactor that routes block-level changes through the existing structural-row pipeline instead of beside it.

Quick rundown of your points:

Parallel models — gone. getBlockTrackedChanges, the {kind} shape, the operationId field, and the blockTrackedChangeAttrSpec helper (plus the dead spreads in table-row.js and paragraph.js) are all deleted. applyHunks now just calls into stampTableRows, so generated and imported revisions go through the same path and carry the same OOXML shape.

Metadata + grouping — handled by stampTableRows. Author/date come from editor.options.user, and revisionGroupId groups multi-row table changes just like native ones.

acceptAll / rejectAll return-value bug — fixed. Block-level fallback dropped; everything routes through dispatchReviewDecision, so block-only changes both mutate the doc and return true. Same applies to the per-id variants.

PR description staleness — fair, will update it next.

Paragraph support — agreed, dropping it from this PR. Separate design as you suggested.

One small new export: enumerateStructuralRowChanges, so downstream consumers can read structural changes through the same pipeline (replaced our parallel block walk on our side).

Net diff: ~600 lines deleted, ~30 added. Smoke-tested end-to-end against our AI-review flow (single/multi-row insert + delete, per-pill accept/reject, mixed accept-all/reject-all). All clean.

Will reach out on discord — thanks again for taking the time on this.

…table they replace

When a tracked-change AI edits an existing table in place, the resulting
proposal table has a different content fingerprint, so it is matched as
remove + insert rather than as the same table. The insert anchor used to
be the end of the last shared base block, which falls back to position 0
when the edited table is the first block — moving the new copy to the
start of the document, far from the original being struck through.

Switch to a lockstep two-pointer walk over base + proposal. When emitting
an insert hunk, anchor at the position of the next unmatched-base entry
(the block the insert is logically replacing), falling back to the end of
the last matched base block only when there is no waiting unmatched base.
Pure inserts and pure removes are unchanged.

Adds regression tests for editing the first table in a multi-table doc
and editing a later table with an intro paragraph.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants